-
Notifications
You must be signed in to change notification settings - Fork 698
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Collection of patches to do with --working-dir #10256
Conversation
70c4f21
to
97ff932
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you audit all uses of runDbProgram
and change them to runDbProgramCwd
when appropriate? For instance I am seeing one usage in Distribution.Simple.GHCJS.installExe
for instance (sigh).
Does it fix any problems in 3.12? If so, would it be backportable as a whole or only some commits? |
It would need to be completely rewritten to be backported, as it's necessarily heavily dependent on #9718. |
The patch doesn't fix any issues present in 3.12. |
97ff932
to
bf42643
Compare
812d1e8
to
3d350de
Compare
It is only on Cabal 3.13 that the symbolic path abstraction was introduced. On CI we only test with Cabal master so the incorrect bound wasn't picked up.
This fixes a simple oversight where the flags were passed without updating the `--working-dir` argument appropiately.
This fixes the --gen-pkg-config to use the symbolic path abstraction, in turn this ensures that we interpret the path appropiately when `--working-dir` is also set.
This refactoring enforces a simple property * We use symbolic paths in Cabal in order to represent that paths to package databases. These paths is relative to the package root. * We use normal filepaths in cabal-install to represent the path to a package database. These are relative to the current working directory. Paths are explicitly converted from one type to the other at the interface of `cabal-install` and `Cabal`, see `setupHsConfigureArgs` for where this happens. In order to achieve this `PackageDB` is abstracted over what the type of filepaths a specific package db points to. ``` type PackageDBX fp = ... | SpecificPackageDB fp | ... ``` If you are using the Cabal library then you probably want to migrate to use `PackageDBCWD` and `PackageDBStackCWD`. ``` type PackageDBCWD = PackageDBX FilePath type PackageDBStackCWD = [PackageDBCWD] ``` Then at the point where you call commands in the `Cabal` library convert these paths into paths relative to the root of the relevant package. The easiest way to do this is convert any paths into an absolute path. This patch fixes a double interpretation issue when the `--working-dir` option was used and package db paths were offset incorrectly.
There are a few places where paths are known to be absolute. This enforces that in the type system by introducing a simple wrapper to `Distribution.Utils.Path`. ``` newtype AbsolutePath (to :: FileOrDir) = AbsolutePath (forall from . SymbolicPath from to) ``` The nice thing about this abstraction is when when a path is unwrapped, due to the universally quantified `from` type, the resulting `SymbolicPath` can be used with any API which expects a path to point `from` a specific directory.
runDbProgram doesn't take into account the working directory (so will normally produce incorrect results when used in `Cabal`). This replaces the last uses which weren't found by testing, they were found by grepping.
When testing `./Setup` only, when `withDirectory` is used, instead of changing into that directory when invoking processes, we now use the `--working-dir` flag and keep a fixed CWD. This will therefore passively test that `--working-dir` is working In addition, it makes it possible to test things easily such as `--working-dir` with a relative path as an argument. `cabal-install` will only invoke `--working-dir` with an absolute path and hence is isolated from any double interpretation issues. Testing against these double interpretation issues is very important as it also prevents over-interpretation of relative paths into absolute paths. Passing absolute paths to tools such as hsc2hs can lead to the build directory leaking into an interface file which leads to non-reproducible results.
Before the previous patches this test failed because an incorrect path was passed to hsc2hs (a preprocessor), it now succeeds :)
3d350de
to
addcd41
Compare
On today's Cabal meeting, we discussed what could let this API-changing PR stay under the radar of 3.14.0.0's release notes. Unanimously people cited the absence of the PR template. We'll try to be more diligent about that. But we also ask the contributors to keep the template (and in particular choose one of the two templates) when submitting PRs. |
There are ways to cut PRs that don't take the template into account, notably the Git integration in VSCode/VSCodium. I think I've encountered a few others but don't recall offhand what they were, just that they involved third party tooling instead of the GitHub website. |
@geekosaur that's a great point, I didn't think about it. I myself have dabbled with Magit Forge, which is an Emacs extension allowing, among other things, to create PRs. Oh well... I guess, the reviewers will have to pay more attention if we want a better future. |
See individual commits for details